Repository navigation
Use assigned workspace tab color for selected sidebar rows - #2569
lawrencecchen wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded color science utilities to compute relative luminance and contrast ratios. Enhanced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
78-138: Consider adding focused regression tests for color-adjustment boundaries.Given this is behavior-critical UI logic, a few unit tests would help lock in: invalid hex handling, no-custom fallback path, and very bright custom colors meeting the readability threshold.
Also applies to: 177-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 78 - 138, Add unit tests covering the color-adjustment boundaries for the sidebar color helpers: write tests that exercise sidebarSelectedWorkspaceCustomBackgroundNSColor and sidebarSelectedWorkspaceReadableBackgroundNSColor (and indirectly sidebarSelectedWorkspaceRelativeLuminance and sidebarSelectedWorkspaceContrastRatio) to assert: 1) invalid hex returns nil (invalid hex handling), 2) absence of a custom color falls back to the expected behavior/path, and 3) extremely bright input colors are iteratively darkened until the contrast with white meets the minimumContrast (4.5) — include assertions on final contrast ratio and that iteration halts within the expected iteration limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 78-138: Add unit tests covering the color-adjustment boundaries
for the sidebar color helpers: write tests that exercise
sidebarSelectedWorkspaceCustomBackgroundNSColor and
sidebarSelectedWorkspaceReadableBackgroundNSColor (and indirectly
sidebarSelectedWorkspaceRelativeLuminance and
sidebarSelectedWorkspaceContrastRatio) to assert: 1) invalid hex returns nil
(invalid hex handling), 2) absence of a custom color falls back to the expected
behavior/path, and 3) extremely bright input colors are iteratively darkened
until the contrast with white meets the minimumContrast (4.5) — include
assertions on final contrast ratio and that iteration halts within the expected
iteration limit.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR wires the per-workspace custom color into the sidebar's selected-row background, applying a WCAG-based contrast-darkening loop so that the selected-state text stays readable over bright custom colors. The fallback chain (custom workspace color → user-set sidebar selection color → accent color) is preserved. Key changes:
Confidence Score: 3/5Functionally adds the custom color to sidebar selection, but the contrast loop assumes white foreground text, which may produce incorrect behavior in light mode where the foreground is dark. The core feature logic is sound for dark mode, but the hardcoded white foreground assumption in sidebarSelectedWorkspaceReadableBackgroundNSColor is a real correctness risk for light-mode appearances. Additionally the unused default parameter is a minor maintenance concern. The change is isolated to sidebar rendering and has no side effects on other subsystems. Sources/ContentView.swift — specifically sidebarSelectedWorkspaceReadableBackgroundNSColor (contrast target) and sidebarSelectedWorkspaceBackgroundNSColor (unused default parameter). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[selectionBackgroundColor] --> B[sidebarSelectedWorkspaceBackgroundNSColor]
B --> C{customHex present?}
C -- Yes --> D[sidebarSelectedWorkspaceCustomBackgroundNSColor]
D --> E[WorkspaceTabColorSettings.displayNSColor]
E --> F[sidebarSelectedWorkspaceReadableBackgroundNSColor]
F --> G{contrast with white < 4.5 AND iter < 12?}
G -- Yes --> H[blend 12% black]
H --> G
G -- No --> I[Return darkened color]
I --> Z[Use as background]
C -- No --> J{sidebarSelectionColorHex present?}
J -- Yes --> K[NSColor hex parse]
K --> Z
J -- No --> L[cmuxAccentNSColor]
L --> Z
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Keep the assigned hue, but darken overly bright custom colors until the | ||
| // existing white selected-state foreground remains readable. | ||
| while sidebarSelectedWorkspaceContrastRatio(between: adjusted, and: NSColor.white) < minimumContrast, | ||
| iteration < 12 { | ||
| guard let darkened = adjusted.blended(withFraction: 0.12, of: .black) else { break } | ||
| adjusted = darkened.usingColorSpace(.sRGB) ?? darkened | ||
| iteration += 1 | ||
| } | ||
|
|
||
| return adjusted | ||
| } |
There was a problem hiding this comment.
The readability loop always measures contrast against NSColor.white:
while sidebarSelectedWorkspaceContrastRatio(between: adjusted, and: NSColor.white) < minimumContrast,This is only correct when the selected-row label text is actually white. In light-mode appearances, the sidebar's selected-state text is often rendered in a dark color (e.g., .labelColor, which resolves to near-black in light mode). Darkening a mid-range color to achieve 4.5:1 against white when the real foreground is dark would overshoot — making the background needlessly dark in light mode.
Consider passing colorScheme into sidebarSelectedWorkspaceReadableBackgroundNSColor and using NSColor.white in dark mode but NSColor.black (or a sampled label color) in light mode so the contrast target matches the actual rendered foreground.
|
|
||
| private func sidebarSelectedWorkspaceReadableBackgroundNSColor(_ color: NSColor) -> NSColor { | ||
| let minimumContrast: CGFloat = 4.5 | ||
| var adjusted = color.usingColorSpace(.sRGB) ?? color | ||
| var iteration = 0 |
There was a problem hiding this comment.
forceBright omitted — potential visual inconsistency in leftRail mode
WorkspaceTabColorSettings.displayNSColor is called here without forceBright:
guard let color = WorkspaceTabColorSettings.displayNSColor(
hex: hex,
colorScheme: colorScheme
) else {Every other call site in TabItemView passes forceBright: activeTabIndicatorStyle == .leftRail (lines 13437–13440, 13445–13448). This means the selection background in leftRail mode starts from the base (un-brightened) color, while the left-rail indicator and the color swatch both use the brightened version. The contrast-darkening loop will then operate on a dimmer starting color and may produce a slightly different shade than expected. If this is intentional (avoiding double-brightness before darkening), a comment explaining the decision would help.
|
Closing this PR because it was created from the wrong GitHub identity. Replacement: #2570. |
This branch was successfully deployed
2 participants
Summary
Testing
Closes #2565
Summary by cubic
Use each workspace tab’s assigned color for the selected sidebar row, falling back to the current highlight when no custom color is set. Automatically darkens bright custom colors to keep the white selected-state text readable in light and dark modes (closes #2565).
Written for commit 9e7d5f3. Summary will update on new commits.
Summary by CodeRabbit